Skip to content

fix(github): bind /builder to the plan the approver selected - #259

Open
islahuddinn wants to merge 2 commits into
theam:mainfrom
islahuddinn:fix/github-builder-plan-binding-233
Open

fix(github): bind /builder to the plan the approver selected#259
islahuddinn wants to merge 2 commits into
theam:mainfrom
islahuddinn:fix/github-builder-plan-binding-233

Conversation

@islahuddinn

Copy link
Copy Markdown

Summary

  • Supersedes older open plan_acceptance proposals when a newer Architect run opens on the same issue, so only one live plan remains in the common case.
  • Refuses bare /builder when multiple live plans exist on an issue (builder_plan_ambiguous).
  • Accepts /builder <proposal-id> (and /codex-builder <proposal-id>) to bind Gate 1 approval to the plan the human actually reviewed.
  • Updates Architect plan publication comments to show the explicit approval command.

Fixes #233

Test plan

  • vitest run test/plan-acceptance.test.ts test/builder-plan-policy.test.ts test/github.test.ts test/github-run-progress.test.ts
  • vitest run test/builder-plan-policy.integration.test.ts (requires Postgres at DATABASE_URL)
  • Manual: run /architect twice on one issue, verify bare /builder is blocked and /builder prop_… approves the intended plan

Made with Cursor

ALVZ93 commented Sep 2, 2026

Copy link
Copy Markdown

I think there is still an atomicity window in ensureArchitectPlanAcceptance worth closing before merge.

Right now supersedeOpenGithubPlanAcceptances(...) runs before the new plan_acceptance proposal is inserted. That creates two failure/race cases:

  1. If the subsequent proposal/event insert fails, the previous valid open proposal has already been cancelled, leaving the issue with no live plan.
  2. Two Architect completions can both run the supersede query before either inserts its own proposal, then both insert successfully and leave two live proposals.

The new ambiguous /builder guard makes case 2 fail closed at approval time, which is good, but it means the "newer plan supersedes older plans" invariant itself is still not atomic.

Could the new proposal creation + open event + superseding of older proposals be done in one DB transaction, with the new proposal inserted first and the older rows cancelled in that same transaction? A regression test that forces the create step to fail (and asserts the previous proposal stays open) would also pin this down.

@islahuddinn
islahuddinn force-pushed the fix/github-builder-plan-binding-233 branch from 3dac681 to f90f7ac Compare September 2, 2026 22:39
islahuddinn and others added 2 commits September 3, 2026 11:44
Supersede older open plan_acceptance proposals when a newer Architect run
opens on the same issue, refuse bare /builder when multiple live plans exist,
and accept `/builder <proposal-id>` for explicit Gate 1 binding.

Fixes theam#233

Co-authored-by: Cursor <cursoragent@cursor.com>
Insert the new plan_acceptance row before cancelling siblings, all under
one issue-scoped transaction lock. A failed create now leaves the previous
live plan open, and concurrent Architect completions serialize instead of
both remaining eligible for /builder.

Co-authored-by: Cursor <cursoragent@cursor.com>
@islahuddinn
islahuddinn force-pushed the fix/github-builder-plan-binding-233 branch from f90f7ac to c7a7c4c Compare September 3, 2026 06:58
@islahuddinn

Copy link
Copy Markdown
Author

Thanks @ALVZ93 — you were right about the window.

supersedeOpenGithubPlanAcceptances now runs after the new plan_acceptance insert, in the same transaction, under an issue-scoped advisory lock (architect-plan-issue:<org>:<repo>:<issue>). Concurrent Architect completions serialize on that lock, so they can no longer both cancel siblings and then both insert.

The regression you asked for is in builder-plan-policy.integration.test.ts: creating the replacement proposal with an invalid action type fails, the transaction rolls back, and the previous live plan stays open. A second test covers the success path (insert first, then cancel older rows) so bare /builder binds to the kept proposal.

Rebased onto current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub /builder approves the newest plan on the issue, not the one the approver read

2 participants